UN-4009 [MISC] Generate and commit the API deployment OpenAPI spec in-repo - #2237
UN-4009 [MISC] Generate and commit the API deployment OpenAPI spec in-repo#2237chandrasekharan-zipstack wants to merge 7 commits into
Conversation
The published Python clients and their generated SDKs are built from a spec of the deployment execute/status endpoints, which until now was produced by a script living outside this repo — so a route or serializer change here could silently invalidate it. The schema annotation for DeploymentExecution now lives next to the view, and `manage.py generate_docstudio_spec` writes specs/docstudio-oss.json. A unit test regenerates and compares, so drift fails in this repo's existing CI tier rather than in a client repo, with no database or extra CI job needed. The generated spec is unchanged from what the external script produced, apart from a root `tags` array — clients had nowhere to read group descriptions from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The committed spec is what published clients are generated from, so the places where it disagreed with the server are places every SDK inherits. - Declare the bearer scheme the endpoints enforce. DRF's unset authentication default was being introspected as a decision and published session and basic auth, which these endpoints do not accept. - Declare the failures a caller has to handle (400/401/403/404/409/429) and describe the 406, so a generated client can branch on them. - Derive the response model from the serializer that builds the response, which drops `workflow_id` -- a field no code path produces. - Stop shadowing `files`: the real field carries the binary annotation, so a change to it now moves the spec. - Drop the MCP operations. MCP speaks JSON-RPC over one POST, so it had no REST shape to describe and was published with guessed responses, no security, and an internal docstring as its description. - Say in the shipped text that a status read is one-shot, and that documents may be supplied as files or presigned URLs. The gate had the same blind spots. It now resolves the real mount instead of comparing against a hand-written copy of it, fails when the generator reports a diagnostic instead of certifying its guess, and asserts the auth scheme and error statuses. Verified by mutation: moving the mount, adding a response field, changing the `files` constraint and dropping the auth annotation each redden the suite, and none of them did before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The deployment endpoints are the public API surface, and the generated clients carry this title into their own documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The spec describes one endpoint today, and five of these tests read it by unpacking a single item or by indexing get and post directly. The first endpoint added turns all five red for no reason, and a GET-only one raises KeyError. They now walk whatever the spec documents. The drift and diagnostics gates are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
|
| Filename | Overview |
|---|---|
| backend/api_v2/openapi_schema.py | Defines client-facing request, response, authentication, operation, and error metadata for the deployment execute and status endpoints. |
| backend/api_v2/management/commands/generate_docstudio_spec.py | Generates a deterministic committed schema and provides a non-writing drift-check mode. |
| backend/api_v2/deployment_spec_urls.py | Selects the deployment route from the served root URL configuration so generated paths retain their real mount prefix. |
| backend/api_v2/tests/test_docstudio_spec.py | Verifies committed-schema drift, route reachability, authentication metadata, operation coverage, failure responses, and response-field compatibility. |
| backend/api_v2/serializers.py | Adds schema metadata that represents multipart uploads as binary without changing request validation behavior. |
| backend/backend/settings/base.py | Configures drf-spectacular as DRF's schema generator and removes the legacy documentation app configuration. |
| backend/pyproject.toml | Replaces drf-yasg with a pinned drf-spectacular dependency to keep committed schema rendering deterministic. |
| specs/docstudio-oss.json | Commits the generated OpenAPI contract consumed by downstream deployment clients. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Runtime[Deployment views and serializers] --> Generator[drf-spectacular SchemaGenerator]
Routes[Deployment spec URLconf] --> Generator
Annotations[OpenAPI annotations] --> Generator
Generator --> Spec[specs/docstudio-oss.json]
Generator --> DriftTest[Schema drift tests]
Spec --> DriftTest
Spec --> Clients[Published client and generated SDK]
Reviews (2): Last reviewed commit: "test: name the downstream repos in the d..." | Re-trigger Greptile
The `docs` app served a redoc UI over a schema drf-yasg introspected with `public=False`, so an anonymous caller saw nothing and no endpoint carried a `@swagger_auto_schema` annotation. Nothing generates or consumes it. Removes the dependency, the `docs` app and its two mounts. The `/doc/` route goes with it; drf-spectacular serves no UI, only the committed spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFSunNN6RKRA1xo6kWkztx
The annotation serializers exist only to shape the published spec, so they sit in `api_v2/openapi_schema.py` rather than in `serializers.py`, where a request-time import of one would look ordinary. `api_deployment_views.py` keeps a single decorator. `deployment_spec_urls.py` now selects mounts from a tuple, so widening the spec to another endpoint is one entry plus its `@extend_schema`. The generated spec is unchanged: component names are all that reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFSunNN6RKRA1xo6kWkztx
…ponse fields The drift test is the only gate, so its message has to reach the person or agent who then has to regenerate the client and the CLI; it now names both repos, as does the management command's `--check`. Adds one binding the annotation could not express by inheritance: the view returns the execution DTO as a dict rather than through `APIExecutionResponseSerializer`, so a renamed DTO field would reach clients as a field the server never sends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFSunNN6RKRA1xo6kWkztx
|
Unstract test resultsPer-group results
Critical paths
|
ritwik-g
left a comment
There was a problem hiding this comment.
Standardized review — verdict: REQUEST CHANGES
Critical: 0 · High: 2 · Medium: 8 · Low: 7 · Lenses run: 17/17
Reviewed against a fixed 17-lens rubric (unstract:standard-review, plugin v0.30.1) at 51c7cd45, diffed from merge-base 6b916eac — 15 files, matching GitHub's count.
The engineering here is careful: the guard against spectacular's guessing, the urlconf that selects the real mount rather than restating it, the failure message naming the downstream repos. Two things to say up front:
- The claim that CI fails on drift is real — I suspected it wasn't and checked.
tests/groups.yaml:85-94collectsbackend/**by glob, and this PR's own CI run showsunit-backendat 1009 passed. - A dedicated security pass (the plugin's vendored prompt, separate from the lenses) found no findings at confidence ≥ 8 — an empty result within its scope, not by exclusion.
/doc/was never inWHITELISTED_PATHS, the committed spec has noserversblock and no secrets, and removing the redoc UI net-reduces attack surface.
Both High findings were confirmed by running the real serializers against the real DTOs in a scratch venv, not by reading. Both reach a generated SDK, and this spec is the artifact Zipstack/unstract-python-client#27 is generated from — so fixing them here will move that PR's generated code.
Unanchored findings
These have no line in this diff to attach to.
- [Medium] [Lens 17]
backend/README.md:178-179now points at a route this PR deletes. "access the API documentation that's auto generated at the backend endpoint/api/v1/doc/" — that route goes withdrf-yasgand thedocsapp (public_urls.py:31,public_urls_v2.py:28). The section now describes an endpoint that 404s, on the page a new backend developer reads first. The only survivingdoc/route isadmin/doc/(Django admindocs), a different mount behindADMIN_ENABLED. Suggest replacing the section with the committed spec and the regeneration command. (Verified directly — one automated sweep reported no surviving references and was wrong.) - PR title — this repo states no title convention in writing (no
CLAUDE.md, noCONTRIBUTINGtitle section, template links out only), so there is nothing to judge against.MISCis the right type regardless: nothing here changes what the product does for a user. - PR body drops template sections, including Dependencies Versions — on the one PR that swaps
drf-yasgfor a pinneddrf-spectacular==0.30.0, that's the section this change most needed. - One description claim is unfalsifiable from this repo: "byte-identical to what the external script produced, apart from the root
tagsarray." The external script isn't here;unstract-python-client#27is where that gets checked.
Open questions
- Is a cloud spec generated from this same command? That decides whether the
hitl_*finding wants a flatexclude_fieldsor a conditional. - Was the
{status, message}ErrorResponseshape a deliberate simplification, or written beforedrf-standardized-errorsbecame the handler?
Assumptions
drf-standardized-errorsis the active handler for these endpoints (verified atbackend/middleware/exception.py:48). If a deployment overridesEXCEPTION_HANDLER, the first finding's severity changes.specs/docstudio-oss.jsonis the OSS artifact only, per its filename andDEFAULT_OUT. If it also feeds cloud, thehitl_*finding becomes High.
Lens checklist (17/17)
1 see findings · 2 see findings · 3 see findings · 4 Clean — dedicated security pass, plus: all published paths match served routes, spec has no servers/secrets, /doc/ was never whitelisted · 5 N/A — no migrations or persisted state · 6 N/A — no concurrency primitives · 7 see findings; consumer half checked separately — endpoint, status codes and wire format are untouched, so unstract-cloud/load_test and unstract-docs are unaffected; the spec's consumers are the unmerged #27 and the CLI · 8 N/A — generation is offline, no external calls added · 9 Clean · 10 Clean · 11 Clean — backend/docs/ fully removed, zero residual drf_yasg references, no docker/chart references, no request-time behaviour change · 12 Clean — the only agent-surface change is @extend_schema(exclude=True) on MCPServerView, documentation-only · 13 see findings · 14 Clean — pinned with a stated reason, canonical package, lockfile consistent, transitive set shrinks · 15 see findings · 16 see findings · 17 see Unanchored
Posted as COMMENT, not REQUEST_CHANGES — the merge decision is yours, not the review's.
|
|
||
| class ErrorResponse(serializers.Serializer): | ||
| status = serializers.CharField(required=False) | ||
| message = serializers.JSONField(required=False, allow_null=True) |
There was a problem hiding this comment.
[High] [Lens 7, 3, 2] — Every error response in the spec has a shape the server never sends
ErrorResponse publishes {status, message}, but 400/401/403/404/409/429 are all raised as APIException and routed through drf_standardized_errors (backend/middleware/exception.py:48), which emits {type, errors:[{code, detail, attr}]} — a disjoint shape. A generated SDK models both documented fields, finds them permanently absent on every failure, and has nothing to read a message from.
{status, message} fits only the hand-built 406/422/500 bodies (api_deployment_views.py:196-228).
Verified — the pinned handler run against this repo's own exceptions:
401 {'type':'client_error','errors':[{'code':'error','detail':'Unauthorized','attr':None}]}
400 {'type':'validation_error','errors':[{'code':'invalid','detail':'You must provide at least one file or presigned URL.','attr':None}]}
Corroborated in-repo by backend/middleware/test_exception.py:41-43 and frontend/src/hooks/useExceptionHandler.jsx:35.
Fix: the repo already ships drf-standardized-errors==0.15.0 with drf_standardized_errors/openapi.py for exactly this — use its AutoSchema as DEFAULT_SCHEMA_CLASS; or minimally redefine ErrorResponse to the handler's shape and keep {status, message} for 406/422/500 only.
| """The execution's identity and, once it has finished, its per-file | ||
| results. | ||
| """ | ||
|
|
There was a problem hiding this comment.
[High] [Lens 7, 3] — error and status_api are published required non-nullable, but every successful 200 sends error: null
ExecutionResponse.__post_init__ (workflow_manager/workflow_v2/dto.py:72) forces falsy error to None, and DRF emits None verbatim for a bare CharField. So the happy path violates the published schema. status_api is null on both error returns in deployment_helper.py:300-307 and :361-367 — the 422 and 500 bodies.
Verified — the real serializer replayed over the real dataclass:
{'execution_status':'PENDING','status_api':'/deployment/...','error':None,'result':None}
A pydantic/Go/Java client raises on the success response.
The comment just above at :52-55 diagnoses this exact hazard for result and fixes it one field over — error is null more often than result is.
Fix: restate both as CharField(required=False, allow_null=True), regenerate.
| class FileResult(serializers.Serializer): | ||
| file = serializers.CharField() | ||
| file_execution_id = serializers.CharField(required=False) | ||
| status = serializers.CharField(required=False) |
There was a problem hiding this comment.
[Medium] [Lens 7] — FileResult declares a metrics the server never emits and omits extracted_text which it does
Metrics live one level down at item["result"]["metrics"] — remove_result_metrics pops it off the inner dict (dto.py:163-169) — so no writer produces a top-level item["metrics"] and file_result.metrics is always empty.
Separately, promote_extracted_text sets item["extracted_text"] (dto.py:143-154) whenever include_extracted_text=true, and that field is absent here — so a typed client drops the payload the caller explicitly opted in for.
Verified — traced both API-result writers (endpoint_v2/destination.py:536-556, endpoint_v2/dto.py:137-146) and grepped every metrics/extracted_text write across backend/ and workers/.
Fix: drop metrics; add extracted_text = CharField(required=False, allow_null=True).
| parameters=DEPLOYMENT_PATH_PARAMETERS, | ||
| request={"multipart/form-data": ExecuteRequest}, | ||
| responses={ | ||
| 200: ExecuteResponse, |
There was a problem hiding this comment.
[Medium] [Lens 7] — execute can return 413/502/504 and arbitrary upstream statuses the spec does not declare
fetch_presigned_file raises 413 on oversize (deployment_helper.py:597-601), 504 on timeout (:665-668), 502 on connection error (:669-674), and — most consequentially — the upstream response's own status verbatim (:659-662, status_code = e.response.status_code).
So an expired S3 signature surfaces as a 403 whose spec description reads "No API key was supplied.", and an S3 404 as "No such active deployment." Undeclared statuses reach a generated client as an unmodelled response.
Fix: declare 413/502/504, and reword the 403/404 descriptions so they don't assert a cause the endpoint can't guarantee. Better still, normalise upstream statuses to a single 502 in fetch_presigned_file so the published contract is closed.
| STATUS_DESCRIPTION = ( | ||
| "Read the result of a previously started execution.\n\n" | ||
| "This read is one-shot: the first call that observes a completed execution " | ||
| "acknowledges it and the stored result is discarded, so every later call " |
There was a problem hiding this comment.
[Medium] [Lens 7, 16] — STATUS_DESCRIPTION tells callers to poll without saying the pending poll answers 422
"Poll while the execution is pending" reads as a 200-returning loop. api_deployment_views.py:230-246 starts response_status at HTTP_422_UNPROCESSABLE_ENTITY and raises it to 200 only on CeleryTaskState.COMPLETED.
Generated SDKs raise on 4xx by default, so the documented polling loop throws on every iteration until completion. The shape is declared (422: StatusResponse) so nothing is strictly wrong — the caller is simply not told the normal path is a non-2xx.
Fix: one sentence — a still-running execution answers 422 with the current status; only a completed one answers 200.
| assert operation["security"] == [{"deploymentKey": []}], f"{method} {path}" | ||
|
|
||
|
|
||
| def test_clients_can_branch_on_every_failure_they_will_see() -> None: |
There was a problem hiding this comment.
[Low] [Lens 13] — Two operation-level tests pass vacuously on an empty spec
Both this and test_operations_require_the_deployment_key (:97-98) loop over _operations(_committed()) with no non-emptiness guard, so a spec whose paths collapsed to {} satisfies them silently.
Today the file as a whole still catches that via :87 and :118 — but incidentally rather than by design. test_the_one_shot_read_is_documented_where_a_client_will_see_it already guards itself with assert reads at :118; these two didn't get the same treatment.
Fix: assert operations after materialising the list, or move the guard into _operations().
| uv run python manage.py generate_docstudio_spec --check # no write, drift is an error | ||
|
|
||
| The generated paths carry ``API_DEPLOYMENT_PATH_PREFIX``, so regenerate in an | ||
| environment that does not override it — the committed artifact describes the |
There was a problem hiding this comment.
[Low] [Lens 13] — The drift gate's outcome depends on ambient API_DEPLOYMENT_PATH_PREFIX, enforced only by this docstring
Generated paths carry settings.API_DEPLOYMENT_PATH_PREFIX (backend/backend/base_urls.py:20), read from the environment at import with a default of deployment. A developer whose environment overrides it sees the drift test fail, regenerates as instructed, and commits a spec whose paths carry a private prefix — the test then passes on the wrong artifact and the downstream SDK repos are generated from it.
Low because nothing in-repo sets the variable (no .env sample, no compose file, and the rig's backend_test_env doesn't pin it) and a prefix change is visible in the specs/ diff.
Fix: assert the rendered path prefix equals the default, or override the setting for the duration of render_spec().
| f"and commit the result.\n\n{DOWNSTREAM}" | ||
| ) | ||
| self.stdout.write(f"{out} is up to date") | ||
| return |
There was a problem hiding this comment.
[Low] [Lens 13] — The --check branch and the write branch are exercised by nothing
--check is invoked by no CI job (ci-test.yaml runs only the tox tiers), no pre-commit hook, and no test. The actual drift gate in CI is test_committed_spec_matches_the_code, which reimplements the same comparison. So the branch the module docstring advertises as the drift command is untested, and the write branch's summary arithmetic at :94-103 is likewise unexercised.
Fix: exercise handle() via call_command (up-to-date, drifted, and write-to-tmp via --out), or drop --check and point the docstring at the pytest gate that actually runs.
(For the record, the drift gate itself is real — tests/groups.yaml:85-94 collects backend/** by glob, and CI shows unit-backend at 1009 passed. Only this branch is dead.)
| GENERATOR_STATS.reset() | ||
| schema = SchemaGenerator(urlconf=URLCONF).get_schema(request=None, public=True) | ||
| if GENERATOR_STATS: | ||
| # spectacular downgrades "unable to guess serializer" to a warning and |
There was a problem hiding this comment.
[Low] [Lens 16] — This comment misstates spectacular's diagnostic severity and what it emits
The comment says spectacular "downgrades 'unable to guess serializer' to a warning and writes a plausible, wrong operation" (restated at test_docstudio_spec.py:55-57). In the pinned 0.30.0, that message is emitted through error() so it lands in _error_cache, not _warn_cache; and serializer resolution returns None, so the operation is published with no request body and a "No response body" response — not a guessed shape.
The guard is correct because it checks both caches. But a maintainer debugging a future failure looks in the wrong bucket and expects a fabricated schema that is never there.
Evidence: drf_spectacular/openapi.py:1269-1273 and :1498-1499.
Also here: docstudio (:23, :25) appears nowhere else in the repo, while the spec's own info.title is "Unstract API" — a maintainer has no path from either name to the other. Consider generate_api_deployment_spec / specs/api-deployment-oss.json, unless it's an established product name.
|
|
||
|
|
||
| # MCP speaks JSON-RPC over one POST, so it has no REST surface worth | ||
| # describing; leaving it in would publish guessed request and response shapes |
There was a problem hiding this comment.
[Low] [Lens 16] — This comment states a consequence the PR's own guard makes impossible
The justification is that "leaving it in would publish guessed request and response shapes to every client generated from the spec." MCPServerView has no serializer_class/get_serializer, so without the exclusion spectacular emits unable to guess serializer into _error_cache — and render_spec(), added in this same PR, raises SpecGenerationFailed on any non-empty GENERATOR_STATS.
Nothing would be published; the command and the drift test would fail loudly. The stated risk is precisely the case the new guard exists to prevent, so the comment misrepresents the safety net a maintainer is standing on.
Fix: "…leaving it in would fail generation, since spectacular cannot guess a serializer for a JSON-RPC APIView and render_spec treats any diagnostic as fatal."



What
The OpenAPI description of the API deployment
execute/statusendpoints is now generated and committed in this repository, and a test fails when it drifts from the code.drf-yasg, which generated nothing anyone read, is removed.Why
The published Python client and the SDK generated for it are built from that description. It was produced by a script living outside this repository, so a route, serializer or response change here could silently invalidate it — the breakage would surface later, in a client repo, against a spec nobody in the PR could see.
How
drf-spectacularadded as a backend dependency;DEFAULT_SCHEMA_CLASSandSPECTACULAR_SETTINGSset insettings/base.py. Both are read only during schema generation — no request-time behaviour changes.api_v2/openapi_schema.pyholds the schema annotation. The introspected schema is wrong in ways that matter to a generated client: a bareFileFieldmaps toformat: uri(right for output, wrong for a multipart upload),result: nullwhile an execution is pending crashes a generated deserialiser withoutallow_null, andoperation_id/tagsdecide the command names and module paths clients expose. It is a module of its own rather than part ofserializers.py, because none of these serializers parses a request or builds a response.api_v2/deployment_spec_urls.py— a urlconf mirroring the real mount. Generating against the included sub-urlconf drops the prefix and produces paths the server does not serve.SPEC_URLCONFSis a tuple: widening the spec to another endpoint is one entry plus its@extend_schema.manage.py generate_docstudio_specwritesspecs/docstudio-oss.jsonwith sorted keys, so the committed file is a usable drift signal.--checkfails instead of writing.api_v2/tests/test_docstudio_spec.pyregenerates and compares, and checks every documented operation resolves to a served URL, declares the deployment key, and documents the failures a client will branch on. It runs in the existingunit-backendgroup — no database, no new CI job. Its failure message names the repos that regenerate from the spec, since propagating the change downstream is the part CI cannot do.drf-yasgand thedocsapp removed. The redoc UI they served was built withpublic=Falseand no endpoint carried a@swagger_auto_schema, so an anonymous caller saw an empty schema. Nothing generated or consumed it.Regenerating
from
backend/, then commit. The test says so in its failure message, along with the client and CLI repos that regenerate from the result.Can this PR break any existing features
The
/doc/route is gone withdrf-yasg, and drf-spectacular serves no UI in its place — the spec is a committed file, not an endpoint. Nothing else changes at request time: the two new settings are read only during schema generation, and no route, serializer or response is modified. The annotation is metadata on the view, not behaviour.Database Migrations
None.
Env Config
None.
Notes on Testing
666 passed, 1 skippedin the backend unit tier this lands in, including the new spec tests. The generated spec is byte-identical to what the external script produced, apart from the roottagsarray — clients had nowhere to read group descriptions from, which is the one deliberate addition.Related Issues or PRs
🤖 Generated with Claude Code
https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ